Fixed encode policy oid - #11019
Conversation
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11019
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
49ac912 to
6755e0b
Compare
|
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11019
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
6755e0b to
47a52ac
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11019
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
|
jenkins retest this please |
47a52ac to
9aee0e8
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11019
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
9aee0e8 to
c07964a
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11019
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
|
|
||
| while (*idx < inSz) { | ||
| if (in[*idx] & 0x80) { | ||
| if (cnt == 0 && in[*idx] == 0x80) |
There was a problem hiding this comment.
The new DecodeOidArc() applies its non-minimal-encoding check (cnt == 0 && in[*idx] == 0x80) and its word32-overflow check (v > (0xFFFFFFFFU >> 7)) unconditionally, i.e. to every arc, not just the first identifier. The strict flag only changes what happens when the input ends mid-continuation. The old loop tolerated both of these for arcs after the first identifier: a leading 0x80 simply contributed 0, and an overflowing shift silently wrapped. Because DecodeCertPolicy() (asn.c:21443) treats any <= 0 return as fatal and returns ASN_PARSE_E, a certificate carrying such a policy OID now fails to parse entirely where it previously parsed. I confirmed this against the built library: {0x2a, 0x80, 0x03} used to decode to "1.2.3" and now returns -144 (ASN_OBJECT_ID_E); {0x2a, 0x90, 0x80, 0x80, 0x80, 0x00} (arc == 2^32) used to render as ".0" and now returns -144. Rejecting is the more correct DER posture and is probably intended, but the PR body only describes an encoder fix plus "improved bounds checking", and the new doxygen note on wc_DecodePolicyOID only mentions the first-identifier strictness and the 2.40-2.47 rendering change.
Recommendation: Confirm the parse-side strictness is intended, then (a) extend the wc_DecodePolicyOID note to cover later arcs
There was a problem hiding this comment.
Confirmed the parse-side strictness is intended and added fixes on the decode route.
| * whatever partial value had accumulated so far. */ | ||
| { | ||
| const byte truncated[] = { 0x81, 0x81 }; | ||
| ExpectIntLT(DecodePolicyOID(decoded, sizeof(decoded), |
There was a problem hiding this comment.
DecodeOidArc()'s strict parameter changes behavior in exactly one place: end-of-input mid-continuation returns ASN_OBJECT_ID_E when strict and ASN_OID_ARC_TRUNCATED (silently dropping the partial arc) when not. Test 16 covers only the strict side ({0x81, 0x81} as a first identifier). Nothing in the new suite exercises a truncated trailing arc, so the if (ret == ASN_OID_ARC_TRUNCATED) break; path in wc_DecodePolicyOID and the return strict ? ... : ASN_OID_ARC_TRUNCATED line are both untested. I verified the intended behavior against the built library: {0x2a, 0x81} returns 3 with "1.2". Similarly, the newly-strict later-arc paths described in the previous finding (non-minimal {0x2a, 0x80, 0x03} and overflow {0x2a, 0x90, 0x80, 0x80, 0x80, 0x00}, both -144) have no assertions, so a future relaxation of that behavior would pass CI silently. The heap != NULL path through wc_EncodePolicyOID (which is what every in-tree caller except the OpenSSL-compat layer uses) is also never exercised.
Recommendation: Add the truncated-trailing-arc, non-minimal-later-arc, overflow-later-arc, and max-word32-arc cases above, plus at least one wc_EncodePolicyOID call with a non-NULL heap so the XMALLOC/XFREE pairing on the heap-hint path is exercised.
There was a problem hiding this comment.
Added suggested test coverage
| len = (word32)XSTRLEN(in); | ||
| if (len == 0 || in[0] == '.' || in[len - 1] == '.') | ||
| return ASN_OBJECT_ID_E; | ||
| if (XSTRSTR(in, "..") != NULL) |
There was a problem hiding this comment.
The new empty-arc rejection calls XSTRSTR(in, ".."). Grepping the tree, this is the only XSTRSTR use in wolfcrypt/src/*.c, and wolfssl/wolfcrypt/types.h:1021-1023 carries an explicit note directly above the definition: "strstr, strncmp, strcmp, and strncat only used by wolfSSL proper, not required for wolfCrypt only". Ports that build with STRING_USER and supply only the wolfCrypt-required X-macros will now fail to compile wolfcrypt/src/asn.c under --enable-certgen --enable-certext, a wolfCrypt-only configuration. The check is also redundant work: the function already walks the whole string in ParseOidArc per token, and the three pre-scans (XSTRLEN, in[0], in[len-1], XSTRSTR) can collapse into one pass.
Recommendation: Replace XSTRSTR with an inline scan so wolfcrypt/src/asn.c does not gain a dependency on a string routine that types.h documents as wolfSSL-proper-only.
There was a problem hiding this comment.
Used inline scan instead
| WOLFSSL_TEST_VIS int wc_EncodePolicyOID(byte *out, word32 *outSz, | ||
| const char *in, void* heap); | ||
| /* Deprecated public API names kept for backwards build compatibility */ | ||
| #define DecodePolicyOID(out, outSz, in, inSz) \ |
There was a problem hiding this comment.
We actually do not need the old macros because they were WOLFSSL_LOCAL. If anywhere else in our code base is using them we should rename those. If customers are using this in a static build (for example) they will need to update their code.
There was a problem hiding this comment.
The old macros have been removed and any remaining internal usages were renamed to use the wc_ prefix.
| @@ -5547,7 +5547,7 @@ static int SetCertificatePolicies(byte *output, | |||
| oidSz = sizeof(oid); | |||
| XMEMSET(oid, 0, oidSz); | |||
|
|
|||
| ret = EncodePolicyOID(oid, &oidSz, input[i], heap); | |||
| ret = wc_EncodePolicyOID(oid, &oidSz, input[i], heap); | |||
There was a problem hiding this comment.
The PR renamed EncodePolicyOID -> wc_EncodePolicyOID at asn_orig.c:5550 but left DecodePolicyOID at asn_orig.c:4227 on the old spelling, relying on the new header macro. Same file, same PR, two conventions. asn_orig.c is #included into asn.c (asn.c:39546) so it compiles either way, but the inconsistency is the reason the compat macros cannot be deleted.
Recommendation: Rename this call site to wc_DecodePolicyOID to match the other in-tree call sites updated by this PR.
| @@ -21229,58 +21229,114 @@ static int DecodeNameConstraints(const byte* input, word32 sz, | |||
| #if defined(WOLFSSL_CERT_EXT) || \ | |||
| defined(OPENSSL_EXTRA) || defined(OPENSSL_EXTRA_X509_SMALL) | |||
|
|
|||
| /* returned by DecodeOidArc() (in place of 0/ASN_OBJECT_ID_E) when the | |||
| * input ends mid-continuation with no terminating byte and strict==0 */ | |||
| #define ASN_OID_ARC_TRUNCATED 1 | |||
There was a problem hiding this comment.
Recommendation: Either rename out of the ASN_* namespace and #undef at the end of the guarded block, or replace the sentinel with an int* truncated out-parameter so the function keeps asn.c's usual 0/negative return contract.
There was a problem hiding this comment.
Replaced the sentinel with an int* truncated out-parameter to maintain the standard 0/negative return contract.
| } | ||
|
|
||
| if ((bufSz = DecodePolicyOID(buf, (word32)bufSz, a->obj + idx, | ||
| if ((bufSz = wc_DecodePolicyOID(buf, (word32)bufSz, a->obj + idx, |
There was a problem hiding this comment.
The doc comment states "String is of the form ... and is always NUL terminated. Truncated when the buffer is too small." The PR tightens wc_DecodePolicyOID's length checks from w > outSz - outIdx to w >= outSz - outIdx and adds a bound check on the first XSNPRINTF that had none, so the exact-fit case that previously produced a silently truncated (and therefore wrong) OID string now returns BUFFER_E, which this function maps to WOLFSSL_FAILURE. The stricter behavior is the right call - a silently shortened OID string is worse than an error, and the missing first-XSNPRINTF check was an out-of-bounds out[outIdx] = 0 write when the first identifier alone overflowed the caller's buffer - but the comment now describes behavior the code no longer has, and it diverges from OpenSSL's OBJ_obj2txt, which truncates and returns the length that would have been needed.
Code:
* String is of the form "1.2.840.113549.1.9.1" and is always NUL
* terminated. Truncated when the buffer is too small.
...
* @return 0 when decoding the object fails.
Recommendation: Update the comment to say the call fails rather than truncates on a short buffer, and note the OpenSSL-compat divergence if it matters for callers of wolfSSL_OBJ_obj2txt(buf, small, obj, 1).
There was a problem hiding this comment.
I've updated the doc comment to explicitly state that the function will fail and return BUFFER_E when the buffer is too small. I also added a note regarding the divergence from OpenSSL's OBJ_obj2txt behavior.
c07964a to
c5b3eed
Compare
e530c44 to
4071edc
Compare
4071edc to
381b1a8
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11019
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 7
7 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
381b1a8 to
0fbefc3
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11019
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfssl-bugs
Failed targets: wolfcrypt-src, wolfssl-src
Findings: 5
5 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
0fbefc3 to
0f22bbb
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11019
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfssl-bugs
Failed targets: wolfcrypt-src, wolfssl-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
| (decodeRet == WC_NO_ERR_TRACE(BUFFER_E))) { | ||
| WOLFSSL_MSG("\tSkipping policy OID that doesn't fit"); | ||
| skipPolicy = 1; | ||
| cert->extCertPoliciesTruncated = 1; |
There was a problem hiding this comment.
🟠 [Medium] extCertPoliciesTruncated not set when the MAX_CERTPOL_NB cap drops policies · API contract violations
extCertPoliciesTruncated is set only for the ASN_OID_ARC_TOO_BIG_E/BUFFER_E skip path. When the while-loop guard extCertPoliciesNb < MAX_CERTPOL_NB (asn.c:21448, asn_orig.c:4281) ends parsing early, entries are dropped with the flag left 0, so wc_CertGetPoliciesTruncated() reports 0 for any cert carrying more than MAX_CERTPOL_NB (2) policies.
Fix: Set cert->extCertPoliciesTruncated = 1 after the loop when it exited on the MAX_CERTPOL_NB cap with input remaining, in both asn.c and asn_orig.c.
| { | ||
| EXPECT_DECLS; | ||
|
|
||
| #if (defined(WOLFSSL_CERT_GEN) && defined(WOLFSSL_CERT_EXT)) \ |
There was a problem hiding this comment.
🔵 [Low] test_wc_EncodePolicyOID guard is looser than the implementation's NO_CERTS guard · API contract violations
wc_EncodePolicyOID and wc_DecodePolicyOID are compiled inside #ifndef NO_CERTS in wolfcrypt/src/asn.c (blocks opened at line 16894 and 27321). The new test's guard omits !defined(NO_CERTS), so an OPENSSL_EXTRA + NO_CERTS build references symbols that do not exist. The sibling test test_wc_EncodePolicyOID_certgen (line 2451) and the mcdc test (test_asn_keys_whitebox.c:1384) both include that condition.
Fix: Add !defined(NO_CERTS) && !defined(NO_ASN) to the #if guard of test_wc_EncodePolicyOID.
| wc_InitDecodedCert(&decoded, der, (word32)certSz, HEAP_HINT); | ||
| ExpectIntEQ(wc_ParseCert(&decoded, CERT_TYPE, NO_VERIFY, NULL), | ||
| 0); | ||
| ExpectIntEQ(decoded.extCertPoliciesNb, 1); |
There was a problem hiding this comment.
⚪ [Info] New BUFFER_E skip branch in DecodeCertPolicy is not exercised · Missing edge-case coverage on a function the PR also changed
DecodeCertPolicy now skips a policy (rather than failing the cert) on two decode results: ASN_OID_ARC_TOO_BIG_E and BUFFER_E (wolfcrypt/src/asn.c:21504). Only the ASN_OID_ARC_TOO_BIG_E arm is covered; the BUFFER_E arm — a policy whose dotted string exceeds MAX_CERTPOL_SZ — is reachable (up to 127 OID content bytes) but untested.
Fix: Add a cert case whose policy OID decodes to more than MAX_CERTPOL_SZ characters and assert the policy is skipped with the truncated flag set.
Description
Fixed a bug in EncodePolicyOID where combined first identifiers (the first two OID arcs) were improperly cast to a single byte when exceeding 127, rather than using the required ITU-T X.690 base-128 continuation encoding. Also added improved bounds checking.
Testing
Added new test coverage to exercise these changes.
Checklist